Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ignore non-DICOM files in a directory #83

Merged
merged 15 commits into from
Dec 13, 2021
Merged

Ignore non-DICOM files in a directory #83

merged 15 commits into from
Dec 13, 2021

Conversation

Dale-Black
Copy link
Contributor

This PR allows dcmdir_parse to work in a directory with non-DICOM-related files. Currently, mac users will run into problems due to .DS_Store and Icon metadata files. This PR modifies isdicom to account for that.

function isdicom(file)
    try
        bytes = read(file, 132)[end-3:end]
        String(bytes) == "DICM"
    catch
        false
    end
end

A test and a non-DICOM image were also added to make sure isdicom is working properly

@testset "isdicom" begin
    answer = DICOM.isdicom("test/testdata/brain.bmp")
    @test answer === false

    fileDX = download_dicom("DX_Implicit_Little_Interleaved.dcm")
    answer2 = DICOM.isdicom(fileDX)
    @test answer2 == true
end

DilumAluthge
DilumAluthge requested changes Nov 27, 2021
View reviewed changes
src/DICOM.jl Outdated Show resolved Hide resolved
Dale-Black and others added 2 commits December 3, 2021 10:56
@Dale-Black @DilumAluthge
Update src/DICOM.jl
ba46466 
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
@notZaki
Create invalid dicom file for testing isdicom()
04785d4
@notZaki
Copy link
Member

notZaki commented Dec 5, 2021 •
edited
Loading

Thanks for catching and fixing this @Dale-Black !

I think the tests are currently failing because the brain.bmp file was not added---likely because the testdata folder is included in .gitignore. An alternative could be to create an invalid during the tests. I'll open a PR on your branch with this approach.

EDIT: I opened a PR on your branch.

@Dale-Black
Copy link
Contributor Author

Sorry this took a while, I was busy with finals. I just merged your PR @notZaki so hopefully that fixes everything. Thanks!

@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #83 (4bca7d7) into master (d1e5567) will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   92.57%   92.85%   +0.28%     
==========================================
  Files           2        2              
  Lines         377      392      +15     
==========================================
+ Hits          349      364      +15     
  Misses         28       28
Impacted Files Coverage Δ
src/DICOM.jl 92.24% <100.00%> (+0.31%) ⬆️
src/DICOMData.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1e5567...4bca7d7. Read the comment docs.

@notZaki
Copy link
Member

notZaki commented Dec 13, 2021

LGTM.
Thanks @Dale-Black !

@notZaki notZaki merged commit cfee5eb into JuliaHealth:master Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DilumAluthge DilumAluthge DilumAluthge requested changes

@notZaki notZaki notZaki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Dale-Black @notZaki @DilumAluthge